Attempt to overwrite without race condition#335
Conversation
|
Thanks @thatch! I'll try and give this a review in the coming days. |
|
Any luck with testing? |
Sorry for the delay -- I was out for a bit, and I'm catching up now. I'll do some testing tonight. |
woodruffw
left a comment
woodruffw
left a comment
There was a problem hiding this comment.
I did some local testing (on Linux) and didn't hit any issues with this.
I also went through the old _secure_open_write flags and confirmed that we shouldn't be missing anything important here -- the old flags should be irrelevant for our mkstemp use (since it defaults to binary mode and O_NOFOLLOW shouldn't be relevant for fresh temporary files).
As such, this LGTM! I'd like @frostming to also do a comb over it though 🙂
(I didn't test this locally on Windows, but my understanding is that os.replace is equally atomic there.)
|
Ping @frostming |
|
@thatch I'll give @frostming another week or so to chime in, otherwise I'll merge and release this next week. |
Co-authored-by: Thomas Grainger <[email protected]>
woodruffw
left a comment
woodruffw
left a comment
There was a problem hiding this comment.
Looks good to me -- I think _write could potentially bail out harder if the underlying write fails rather than complete the rename, but that's worth thinking about separately.
3 participants
As explained in #332, there was previously a small window of time where the file is deleted before its new contents get written. Because reads don't happen with the lock held, this resulted in an empty-body cache hit when it shouldn't.
Fixes #332